Skip to content

Conversation

@X9VoiD
Copy link

@X9VoiD X9VoiD commented Jul 4, 2025

Related GitHub Issue

Closes: #5393

Roo Code Task Context (Optional)

Description

  • Rewrote listFiles to use git ls-files for git repos (respects all .gitignores)
  • Added fallback to manual file walk with ignore package for non-git repos
  • Added comprehensive integration tests for nested .gitignore scenarios
  • Improved test coverage with new unit tests for edge cases

The new implementation:

  1. Handles nested .gitignore files correctly
  2. Maintains compatibility with non-git projects

Test Procedure

list-files.spec.ts have been updated to properly test

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Testing: New and/or updated tests have been added to cover my changes (if applicable).
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos

Documentation Updates

Additional Notes

Get in Touch


Important

Fix and optimize listFiles to respect .gitignore using git ls-files and add comprehensive tests.

  • Behavior:
    • listFiles now uses git ls-files for git repos, respecting .gitignore.
    • Falls back to manual file walk for non-git repos using ignore package.
    • Optimizes recursion to skip ignored directories.
  • Tests:
    • Added integration tests in list-files.integration.spec.ts for nested .gitignore scenarios.
    • Updated unit tests in list-files.spec.ts for edge cases and special directories.
  • Misc:
    • Refactored list-files.ts to improve performance and maintainability.

This description was created by Ellipsis for 4273b04. You can customize this summary. It will automatically update as commits are pushed.

@X9VoiD X9VoiD requested review from cte, jr and mrubens as code owners July 4, 2025 14:44
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Jul 4, 2025
@X9VoiD X9VoiD changed the title Fix list files Fix list_files returning .gitignore'd directories Jul 4, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jul 4, 2025
@daniel-lxs
Copy link
Member

Thanks for working on this fix! The issue you're addressing is definitely valid - we should respect .gitignore files throughout the repository hierarchy, not just in the current directory.

I noticed we already have simple-git as a dependency in the project. Have you considered using its built-in gitignore functionality instead? It has a checkIgnore() method that handles all the git traversal logic internally. This could simplify the implementation significantly.

For example, something like:

import simpleGit from 'simple-git';

const git = simpleGit(repoRoot);
const ignoredFiles = await git.checkIgnore(filePaths);

This would eliminate the need for manual directory traversal, finding the git root, and parsing multiple .gitignore files. The simple-git library already handles all of that complexity for us.

What do you think about this approach? It might make the code more maintainable while achieving the same goal.

@daniel-lxs daniel-lxs moved this from Triage to PR [Draft / In Progress] in Roo Code Roadmap Jul 4, 2025
@daniel-lxs daniel-lxs marked this pull request as draft July 4, 2025 15:29
@hannesrudolph hannesrudolph added PR - Draft / In Progress and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Jul 4, 2025
@X9VoiD X9VoiD changed the title Fix list_files returning .gitignore'd directories Fix list_files returning .gitignore'd directories and optimize Jul 5, 2025
- Rewrote listFiles to use git ls-files for git repos (faster and respects all .gitignores)
- Added fallback to manual file walk with ignore package for non-git repos
- Optimized recursion to avoid traversing ignored directories
- Added comprehensive integration tests
- Improved test coverage with new unit tests for edge cases
- Fixed path normalization to ensure consistent forward slashes

The new implementation:
1. Handles nested .gitignore files correctly
2. Provides significant performance improvements in large repos
3. Maintains compatibility with non-git projects
4. Ensures consistent behavior across environments

A caveat of this is that directories by themselves are not reported anymore if they don't contain any files.
@X9VoiD X9VoiD marked this pull request as ready for review July 6, 2025 13:15
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jul 6, 2025
@dosubot dosubot bot removed the size:XL This PR changes 500-999 lines, ignoring generated files. label Jul 6, 2025
@dosubot dosubot bot added the size:XXL This PR changes 1000+ lines, ignoring generated files. label Jul 6, 2025
@X9VoiD
Copy link
Author

X9VoiD commented Jul 6, 2025

@daniel-lxs I updated the code to use simple-git as per suggestion. It is now using git ls-file as primary method of listing files. If workspace is not git repo, it will use the previous ripgrep implementation with filtering done by ignore. I also added a final fallback on that very slim chance that ripgrep binary goes missing.

Overall, I think these changes + the added unit and integration tests make list-files very robust.

@daniel-lxs daniel-lxs moved this from PR [Draft / In Progress] to PR [Needs Prelim Review] in Roo Code Roadmap Jul 7, 2025
@daniel-lxs
Copy link
Member

Hey @X9VoiD, really appreciate your contribution! It looks like the fix in #5394 ends up covering the same issue and is a bit more aligned with how we want to handle it. Sorry, that's my bad for now checking that PR before asking for changes on yours - thanks again for taking the time to work on this!

@daniel-lxs daniel-lxs closed this Jul 9, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Prelim Review] to Done in Roo Code Roadmap Jul 9, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jul 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working PR - Needs Preliminary Review size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

list_files incorrectly returns .gitignore'd directories

3 participants